-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Alerting][Connectors] Refactor Jira: Generic Implementation (phase one) #73778
Conversation
1750196
to
a003995
Compare
904c152
to
4f1f865
Compare
4f1f865
to
1342bf5
Compare
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
Pinging @elastic/siem (Team:SIEM) |
72a1f83
to
a3eb0ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! I've noted some things that I think will need to be changed, mainly regarding schema.maybe()
usage and the rename of the config field.
Note, I haven't looked at the UI code yet, figured I'd post what I have so far before waiting to review that.
x-pack/plugins/actions/server/builtin_action_types/jira/schema.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/actions/server/builtin_action_types/jira/case_schema.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/actions/server/builtin_action_types/jira/schema.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/actions/server/builtin_action_types/servicenow/api.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/case/server/routes/api/cases/configure/get_connectors.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/actions/README.md
Outdated
| Property | Description | Type | | ||
| --------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------- | | ||
| apiUrl | ServiceNow instance URL. | string | | ||
| incidentConfiguration | Case configuration object. The object should contain an attribute called `mapping`. A `mapping` is an array of objects. Each mapping object should be of the form `{ source: string, target: string, actionType: string }`. `source` is the Case field. `target` is the ServiceNow field where `source` will be mapped to. `actionType` can be one of `nothing`, `overwrite` or `append`. For example the `{ source: 'title', target: 'short_description', actionType: 'overwrite' }` record, inside mapping array, means that the title of a case will be mapped to the short description of an incident in ServiceNow and will be overwrite on each update. | object _(optional)_ | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like you are renaming an existing config value of casesConfiguration
to incidentConfiguration
. If you do that, any existing actions with casesConfiguration
will be broken - they won't validate anymore.
I don't believe we had to deal with this yet, but I think the way to do it would be like the way we treat kibana.yml config values that we "rename". Basically, you need to deprecate the old one, and not delete it. In code that checks the value, you will potentially have to check both (use the new one unless it's not set, otherwise use the old one). @mikecote other thought?
I'm not sure if we would support some way for these to be migrated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @pmuellr, I saw the same rename got done in ServiceNow too. From what I see, the impact is that the connector would "lose" the field mappings because incidentConfiguration
would be empty for existing ServiceNow connectors (possible bug in Cases app?) and the user would have to re-configure the mappings. I'm not sure how it would validate when updating 🤔
One way I see this being fixed since we're not GA yet is with a saved object migration in the actions plugin that moves the value from casesConfiguration
to incidentConfiguration
(including decrypt / encrypt process). In a post GA world, @pmuellr is correct and we'd probably have to think of a non-breaking way to support both until the next major where possible.
I'm indifferent what approach is done (migrate or revert to casesConfiguration
or make users re-configure mappings). Since there's a phase 2 to remove this value from connectors, we could always skip it until the field gets removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pmuellr @mikecote I followed ServiceNow PR example. As mapping for the moment is fixed to a default one (only thought the UI), creating or updating a case or validating the mapping will not be a problem. What Cases will loose is the appended text (Example: A title in Jira (created at 2020-04-22T18:18:03.896Z by Christos Nasikas)
) in titles and descriptions. I totally agree with what you are saying but I think it's ok if some users lose temporarily that functionality as in phase 2 we gonna append this information beforehand in Cases. I think the easiest one is to revert back to casesConfiguration
and lose that functionality only for ServiceNow
. @XavierM What are your thoughts on that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of today, this mapping is static meaning nobody can change it. So we can always default back but I think it will be better to do a migration. @cnasikas I can show you how to do it if you do not know, but this one seems pretty simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could deal with a migration today, since the action is in the actions plugin, but in the future when we have actions NOT in the actions plugin, it's not clear to me if they can or should be doing migrations against the action SO's. Basically, in the future, we may need to have a per-actionType migration story that could be used outside of the actions plugin. Actually makes me wonder if every actionType (and alertType) should be their own saved object, or something, to make lifecycle management like this a little more straight-forward.
@@ -123,7 +126,8 @@ export default function jiraTest({ getService }: FtrProviderContext) { | |||
config: { | |||
apiUrl: jiraSimulatorURL, | |||
projectKey: mockJira.config.projectKey, | |||
casesConfiguration: mockJira.config.casesConfiguration, | |||
incidentConfiguration: mockJira.config.incidentConfiguration, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regarding the rename of casesConfiguration
- since actions created with the old config value could already exist from the previous release, should probably have a test for that. Might be hard to test for. Especially hard since the field is part of AAD so you can't just overwrite the field with the generic saved object API. Probably a case for es-archiver, sorry to say. another one that @mikecote may have some input on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ es archiver is probably best in this scenario but there may be issues where it erases other data the test suite relies on (users / spaces). @gmmorris is going through the exact same issue in the RBAC work, maybe a change he's working on will help here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think based on my note above: #73778 (comment). I'd be +1 on reverting it back to casesConfiguration
since we know the field is going away soon and we don't have to worry about a migration or tests to cover this.
x-pack/test/alerting_api_integration/basic/tests/actions/builtin_action_types/jira.ts
Show resolved
Hide resolved
11f824b
to
627a3f2
Compare
* master: (68 commits) a11y tests on spaces home page including feature control (elastic#76515) [ML] Transforms list: persist pagination through refresh interval (elastic#76786) [ML] Replace all use of date_histogram interval with fixed_interval (elastic#76876) [Timelion] Update timelion deprecation links (elastic#77008) [Security Solution] Refactor Network Details to use Search Strategy (elastic#76928) Upgrade elastic charts to 21.1.2 (elastic#76939) [Alerting][Connectors] Refactor Jira: Generic Implementation (phase one) (elastic#73778) [Snapshot & Restore] fix pre existing policy with no existing repository (elastic#76861) Update saved object management UI text (elastic#76826) [Form lib] Add validations prop to UseArray and expose "moveItem" handler (elastic#76949) [Logs UI] Use fields api in log stream (elastic#76919) [UI Metrics] Support multi-colon keys (elastic#76913) [APM] Script for creating functional test archive (elastic#76926) [ENDPOINT] First version of the trusted apps list. (elastic#76304) Correct field for rum page url (elastic#76916) [Security Solution] Fix redirect properly old SIEM App routes (elastic#76868) Bump http-proxy from 1.17.0 to 1.18.1 (elastic#76924) [RUM Dashboard] Visitor breakdown usability (elastic#76834) [Search] Add a new advanced setting searchTimeout (elastic#75728) [DOCS] Adds timelion deprecation to new visualize docs structure (elastic#76959) ...
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
async chunks size
miscellaneous assets size
page load bundle size
distributable file count
History
To update your PR or re-run it, just comment with: |
API changes for creating a Jira connector:
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
Summary
This PR refactors the Jira connector to be generic. Specifically:
issueTypes
andfieldsByIssueType
UI:
API
Push incident:
Pushes an incident to Jira
Endpoint:
api/actions/action/<action_id>/_execute
Method:
POST
Payload:
Response
Get issue types:
Get issue types of a specific project (
projectKey
is configured at the creation of the connector).Endpoint:
api/actions/action/<action_id>/_execute
Method:
POST
Payload:
Response
Get fields of issue type:
Get the fields of a specific issue type.
Endpoint:
api/actions/action/<action_id>/_execute
Method:
POST
Payload:
Response
Checklist
Delete any items that are not applicable to this PR.
For maintainers
Resolves #56426